Automated Test: sms-retry-enhanced #372
Conversation
* add retry count to workflow reminder * add logic to for retry count --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com> Co-authored-by: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR introduces retry tracking for failed SMS reminders in workflows. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts`:
- Around line 31-43: The OR branch currently deletes any reminder with
retryCount > 1 regardless of type or schedule; narrow it so the retryCount
condition only applies to failed SMS reminders without a scheduledDate. Update
the second OR object to include method: WorkflowMethods.SMS and a constraint
that scheduledDate is absent/unscheduled (e.g., null or missing) in addition to
retryCount > 1 so only unscheduled SMS retries are targeted.
- Around line 178-186: The current updates to retryCount use a read-modify-write
pattern (retryCount: reminder.retryCount + 1) which can lose increments under
concurrent cron runs; change both prisma.workflowReminder.update calls that
reference reminder.retryCount to use Prisma's atomic increment API (data: {
retryCount: { increment: 1 } }) so updates are applied atomically via
prisma.workflowReminder.update rather than relying on the stale
reminder.retryCount value.
| OR: [ | ||
| { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Scope retry cleanup to SMS and unscheduled reminders only.
The new OR branch (retryCount > 1) is not constrained to method: SMS, and also deletes scheduled reminders with retries, which can remove unrelated/valid reminders. Consider scoping the delete to SMS and unscheduled failed retries.
🔧 Suggested fix
- await prisma.workflowReminder.deleteMany({
- where: {
- OR: [
- {
- method: WorkflowMethods.SMS,
- scheduledDate: {
- lte: dayjs().toISOString(),
- },
- },
- {
- retryCount: {
- gt: 1,
- },
- },
- ],
- },
- });
+ await prisma.workflowReminder.deleteMany({
+ where: {
+ method: WorkflowMethods.SMS,
+ OR: [
+ {
+ scheduledDate: {
+ lte: dayjs().toISOString(),
+ },
+ },
+ {
+ scheduled: false,
+ retryCount: {
+ gt: 1,
+ },
+ },
+ ],
+ },
+ });🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 31 -
43, The OR branch currently deletes any reminder with retryCount > 1 regardless
of type or schedule; narrow it so the retryCount condition only applies to
failed SMS reminders without a scheduledDate. Update the second OR object to
include method: WorkflowMethods.SMS and a constraint that scheduledDate is
absent/unscheduled (e.g., null or missing) in addition to retryCount > 1 so only
unscheduled SMS retries are targeted.
| } else { | ||
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd scheduleSMSReminders.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 206
🏁 Script executed:
wc -l packages/features/ee/workflows/api/scheduleSMSReminders.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 152
🏁 Script executed:
cat -n packages/features/ee/workflows/api/scheduleSMSReminders.ts | sed -n '170,200p'Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1028
🏁 Script executed:
cat -n packages/features/ee/workflows/api/scheduleSMSReminders.ts | head -100Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 4333
🏁 Script executed:
cat -n packages/features/ee/workflows/api/scheduleSMSReminders.ts | sed -n '1,70p' | tail -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 677
🏁 Script executed:
rg "increment:" packages/features/ee/workflows/ -A 2 -B 2Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 508
🏁 Script executed:
rg "retryCount.*increment" --type ts --type tsxRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 117
🏁 Script executed:
rg "increment:" -t ts -A 1 -B 1 | head -50Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1680
🏁 Script executed:
sed -n '1,50p' packages/features/tasker/repository.ts | grep -A 10 "increment"Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 72
🏁 Script executed:
rg -n "attempts.*increment" packages/features/tasker/repository.ts -B 5 -A 5Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 520
🏁 Script executed:
cat -n packages/features/ee/workflows/api/scheduleSMSReminders.ts | sed -n '160,200p'Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1313
Replace retryCount increment with Prisma atomic operation to prevent lost updates in concurrent cron execution.
retryCount: reminder.retryCount + 1 can lose updates when multiple cron instances process the same reminder concurrently. Both instances read the same value, increment it, and persist the same final count instead of proper sequential increments. Use Prisma's atomic increment.
This occurs at lines 184 and 195.
Suggested fix
data: {
- retryCount: reminder.retryCount + 1,
+ retryCount: { increment: 1 },
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| await prisma.workflowReminder.update({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| data: { | |
| retryCount: reminder.retryCount + 1, | |
| }, | |
| }); | |
| } else { | |
| await prisma.workflowReminder.update({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| data: { | |
| retryCount: { increment: 1 }, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 178
- 186, The current updates to retryCount use a read-modify-write pattern
(retryCount: reminder.retryCount + 1) which can lose increments under concurrent
cron runs; change both prisma.workflowReminder.update calls that reference
reminder.retryCount to use Prisma's atomic increment API (data: { retryCount: {
increment: 1 } }) so updates are applied atomically via
prisma.workflowReminder.update rather than relying on the stale
reminder.retryCount value.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit